Skip to content

Conversation

@navyansh007
Copy link
Contributor

resolves #7524

  • Read, understood, and followed the [contributing guidelines][contributing].

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: passed
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: passed
  - task: lint_c_examples
    status: passed
  - task: lint_c_benchmarks
    status: passed
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@stdlib-bot stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Oct 2, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Oct 2, 2025

Coverage Report

Package Statements Branches Functions Lines
math/base/special/sici $\color{green}874/874$
$color{green}+100.00%$
$\color{green}72/72$
$color{green}+100.00%$
$\color{green}14/14$
$color{green}+100.00%$
$\color{green}874/874$
$color{green}+100.00%$

The above coverage report was generated for the changes in this PR.

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@kgryte kgryte changed the title feat: added C implementation for math/base/special/sici feat: added C implementation for math/base/special/sici Oct 2, 2025
@kgryte kgryte requested a review from anandkaranubc October 2, 2025 19:46
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: passed
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
Signed-off-by: Gunj Joshi <[email protected]>
Signed-off-by: Gunj Joshi <[email protected]>
@gunjjoshi
Copy link
Member

@navyansh007 You'll need to update README.md, which is currently missing from this PR. For reference, you can have a look at this as an example.

}

*si = s;
*ci = STDLIB_CONSTANT_FLOAT64_EULERGAMMA + log( ax ) + c;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navyansh007 You can use @stdlib/math/base/special/ln here, instead of using log() from math.h.

@navyansh007
Copy link
Contributor Author

Thanks for your feedback @gunjjoshi. Going to work on all of those feedbacks.

navyansh007 and others added 3 commits October 29, 2025 09:34
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: missing_dependencies
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@navyansh007
Copy link
Contributor Author

Hey @kgryte @gunjjoshi
I have implemented all the changes suggested by @gunjjoshi in the sici function's C implementation. Feel free to let me know if any further modifications are required.

@gunjjoshi gunjjoshi self-requested a review October 29, 2025 04:16
Comment on lines +45 to +46
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",

We do not need these now, as we're using stdlib_base_sincos.

Comment on lines +67 to +68
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",

Comment on lines +89 to +90
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@stdlib/math/base/special/cos",
"@stdlib/math/base/special/sin",

'encoding': 'utf8'
};

// Generate JavaScript files:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Generate JavaScript files:

Let's not keep this, as none of the other packages such as exp, atan, etc. have such comments. The initialization of opts and copts should be enough to have a visual separation between the generation of JS and C files.

str = header + compile( GD8 );
writeFileSync( fpath, str, opts );

// Generate C files:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Generate C files:

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*/
*
*
* ## Notice
*
* The original C code, long comment, copyright, license, and constants are from [Cephes]{@link http://www.netlib.org/cephes}. The implementation follows the original, but has been modified according to project conventions.
*
* ```text
* Copyright 1984, 1987, 1989 by Stephen L. Moshier
*
* Some software in this archive may be from the book _Methods and Programs for Mathematical Functions_ (Prentice-Hall or Simon & Schuster International, 1989) or from the Cephes Mathematical Library, a commercial product. In either event, it is copyrighted by the author. What you see here may be used freely but it comes with no support or guarantee.
*
* Stephen L. Moshier
* moshier@na-net.ornl.gov
* ```
*/

Let's preserve the full header comment from main.js.

@gunjjoshi
Copy link
Member

@navyansh007 Would you mind replacing the use of sin and cos in main.js by sincos as well, as per #8187 (comment)?

Comment on lines +296 to +297
* @param si output pointer for sine integral
* @param ci output pointer for cosine integral
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param si output pointer for sine integral
* @param ci output pointer for cosine integral
* @param si destination to store the sine integral
* @param ci destination to store the cosine integral

Following what we have in sincos: https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/sincos/src/main.c#L97

* stdlib_base_sici( 3.0, &si, &ci );
*/
void stdlib_base_sici( const double x, double *si, double *ci ) {
double sgn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep sgn as an 8 bit integer, instead of declaring it as double.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that would then require us to make changes at all other places in this function which use sgn, such as replacing sgn = -1.0 by sgn = -1. The cephes implementation too, doesn't keep sgn as double.

@gunjjoshi gunjjoshi added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Math Issue or pull request specific to math functionality. Needs Changes Pull request which needs changes before being merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: Add C implementation for @stdlib/math/base/special/sici

4 participants